propagate method metadata to keyword sorter methods #45041
Conversation
|
Do we want to generalize this to other
julia> lwr = Meta.lower(@__MODULE__, quote
Base.@constprop :aggressive function foo(pos; kw)
pos + kw
end
end);
julia> (lwr.args[1].code[16].args[3]::Core.CodeInfo).constprop
0x01
julia> (lwr.args[1].code[24].args[3]::Core.CodeInfo).constprop # kwsorter
0x00
Yes, I also found that |
Yes please (see #45050). The inability to block constprop on certain "derived" methods is a substantial contribution to latency in packages that have otherwise been carefully TTFX-optimized. I've even added workarounds in some cases, e.g., https://github.com/JuliaImages/ImageView.jl/blob/786fbf584b39387d2d3b5964e4cb40fcb4a3daf3/src/ImageView.jl#L743-L751 |
aviatesk
force-pushed
the
jb/inlinekwsort
branch
2 times, most recently
from
0635759 to
e4fe26d
Compare
There was a problem hiding this comment.
I rebased this PR and also added more supports for propagating other metadata to keyword sorter methods too.
I also added test cases and now all the cases reported in #45050 should be covered by this PR.
Although I'm sure this PR is working, I'd like to have a review from others especially on flisp code, since I'm not very familiar with the codebase.
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
aviatesk
force-pushed
the
jb/inlinekwsort
branch
2 times, most recently
from
2c8ce32 to
a4e275b
Compare
aviatesk
changed the title
| @@ -562,6 +562,9 @@ | |||
| ,(if (any kwarg? pargl) (gensy) UNUSED) | |||
| (call (core kwftype) ,ftype)) ,kw ,@pargl ,@vararg) | |||
| `(block | |||
| ;; propagate method metadata to keyword sorter | |||
| ,@(map propagate-method-meta (filter meta? prologue)) | |||
There was a problem hiding this comment.
Should filter out expressions that end up empty, (meta).
There was a problem hiding this comment.
(filter meta? prologue) should already filter out the empty (meta) case?
Line 527 in a4e275b
| (define (propagate-method-meta e) | ||
| `(meta ,@(filter (lambda (x) | ||
| (or (method-meta-sym? x) | ||
| (and (pair? x) (eq? (car x) 'purity)))) |
There was a problem hiding this comment.
For purity we can only keep the ones that are true of keyword sorters. Safer not to propagate them at all. But I think the following apply to keyword sorters:
- consistent
- effect_free
- terminates_globally
- notaskstate
But please check me on that!
There was a problem hiding this comment.
I think keyword sorters are usually:
:consistent:effect_free:terminates_locally(:terminates_globallyrequires that of the body function itself):notaskstate
but they are not :nothrow (since they can throw any of TypeError/UndefKeywordError/MethodError).
Well, I'd argue it is okay to propagate the effects assumed by Base.@assume_effects to keyword sorters, since the point of using the macro to override the conclusion that is otherwise derived by the effect analysis.
If we write
Base.@assume_effects :total specific(A::Type, B::Type; v::Number=1) =
v * one(ccall(:jl_type_morespecific, Cint, (Any, Any), A, B) ≠ 0 ? A : B)then IMO we'd like to assume both specific(Int, Integer) and specific(Int, Integer; v=42) are :total (and therefore :nothrow also)?
aviatesk
force-pushed
the
jb/inlinekwsort
branch
from
a4e275b to
8f80df8
Compare
|
@JeffBezanson Can I request your review on this again? I'd like to move this forward once CI runs green. |
aviatesk
force-pushed
the
jb/inlinekwsort
branch
from
8f80df8 to
48707d3
Compare
Co-authored-by: Shuhei Kadowaki <[email protected]>
Co-authored-by: Shuhei Kadowaki <[email protected]>
Arguably, if a method is declared
@inlinewe should propagate that to all the methods we generate for it, including the keyword sorter. However, AFAICT #45028 still needs a@constpropdeclaration to function.